Skip to content

ffi: validate 'void' as parameter type in getFunction and getFunctions#63504

Open
Anshikakalpana wants to merge 1 commit into
nodejs:mainfrom
Anshikakalpana:ffi-void-parameter
Open

ffi: validate 'void' as parameter type in getFunction and getFunctions#63504
Anshikakalpana wants to merge 1 commit into
nodejs:mainfrom
Anshikakalpana:ffi-void-parameter

Conversation

@Anshikakalpana
Copy link
Copy Markdown
Contributor

@Anshikakalpana Anshikakalpana commented May 23, 2026

Fixes: #63461

Passing 'void' as a parameter type in getFunction or getFunctions triggers an internal assertion instead of a user-friendly error.

In C, void in a parameter list means a function takes no arguments, expressed in Node.js FFI as an empty array []. There is no valid use case for 'void' as a parameter type.

Fix:
add early validation in getFunction and getFunctions that throws ERR_INVALID_ARG_VALUE when 'void' appears in the parameters array, before wrapWithSharedBuffer is called.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 23, 2026
@Anshikakalpana Anshikakalpana mentioned this pull request May 23, 2026
Open
@ShogunPanda
Copy link
Copy Markdown
Contributor

ShogunPanda commented May 23, 2026

No, this is wrong. Don't validate at JS level but within C++ level please.

@Anshikakalpana Anshikakalpana force-pushed the ffi-void-parameter branch 2 times, most recently from 455f25e to 06410ff Compare May 23, 2026 10:19
@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

Anshikakalpana commented May 23, 2026

No, this is wrong. Don't validate at JS level but within C++ level please.

Moved validation into the native FFI signature parsing layer in src/ffi/types.cc as suggested. Also removed the temporary JS-side validation.

ShogunPanda
ShogunPanda approved these changes May 23, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 23, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/73625/

@codecov
Copy link
Copy Markdown

codecov Bot commented May 23, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (3c2d2e3) to head (43c1eb8).
⚠️ Report is 179 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #63504      +/-   ##
==========================================
+ Coverage   90.04%   90.32%   +0.28%     
==========================================
  Files         713      730      +17     
  Lines      224484   234214    +9730     
  Branches    42430    43926    +1496     
==========================================
+ Hits       202134   211559    +9425     
- Misses      14156    14369     +213     
- Partials     8194     8286      +92
Files with missing lines Coverage Δ
src/ffi/types.cc 49.67% <100.00%> (+0.76%) ⬆️

... and 151 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ShogunPanda
Copy link
Copy Markdown
Contributor

ShogunPanda commented May 24, 2026

CI failures are relevant

@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

Anshikakalpana commented May 24, 2026 •
edited
Loading

CI failures are relevant

Hi @ShogunPanda, the Jenkins failures are from the old commit 06410ff (confirmed via COMMIT_SHA_CHECK in the job parameters). GitHub Actions on the current commit 618bed6 are fully green. Could you please trigger a fresh CI run?

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 24, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/73650/

@Anshikakalpana
ffi: validate 'void' as parameter type in getFunction and getFunctions
43c1eb8 
Fixes: nodejs#63461
Signed-off-by: Anshikakalpana <anshikajain196872@gmail.com>
@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

Anshikakalpana commented May 24, 2026

Fixed the Windows failure — DynamicLibrary(null) rejects null on Windows. Now using kernel32.dll on Windows and null on Unix. Sorry for the multiple runs!

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 24, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/73656/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShogunPanda ShogunPanda ShogunPanda approved these changes

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ffi: 'void' as parameter type triggers internal assertion

3 participants

@Anshikakalpana @ShogunPanda @nodejs-github-bot